Skip to content

feat(screenshot): skip screenshot when preview URL returns non-2xx#289

Merged
krokoko merged 5 commits into
aws-samples:mainfrom
isadeks:feat/287-screenshot-network-status
Jun 10, 2026
Merged

feat(screenshot): skip screenshot when preview URL returns non-2xx#289
krokoko merged 5 commits into
aws-samples:mainfrom
isadeks:feat/287-screenshot-network-status

Conversation

@isadeks

@isadeks isadeks commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Closes #287.

Stacked on PR #241 (the screenshot pipeline). The diff against `main` carries the full pipeline as ancestors — once #241 lands, this PR's net diff collapses to ~340 lines.

Summary

Before: the processor only checked `Page.navigate`'s `errorText`. An HTTP 4xx/5xx response or a redirect that doesn't resolve cleanly navigates "successfully" from CDP's perspective, and a 404/503/etc. PNG gets posted as if it were the deployed app — exactly what krokoko's PR-241 review flagged ("a 404/login PNG gets posted as if it were the app").

After: enable the `Network` CDP domain before navigating; tap the raw `message` stream for `Network.responseReceived` events; record the latest `type==='Document'` response status for the navigated frame. After `Page.loadEventFired` + the 2s settle, throw if the captured status is non-2xx. The processor's existing `catch` swallows the error and skips the comment cleanly.

Auth walls that return 200 (e.g. Vercel deployment protection) are out of scope — caller-side fix; the screenshots guide already documents that.

Implementation notes

  • Listener is wired via `ws.on('message', ...)` directly (multi-fire), not the existing `eventWaiters` one-shot pattern, since redirect chains can fire multiple Document responses
  • `mainDocumentFrameId` is captured from `Page.navigate`'s response and used to filter subsequent Document events to the navigated frame (so an iframe's Document response can't masquerade)
  • If `Network.responseReceived` never fires (defensive, e.g. service variant that doesn't emit Network events), falls through to capture optimistically — pre-feat(screenshot): skip screenshot when preview URL returns non-2xx #287 behaviour, no regression

Test plan

  • 6 new unit tests in `cdk/test/handlers/shared/agentcore-browser.test.ts`:
  • CDK compile clean
  • eslint clean (mutation guard)
  • Smoke on dev: temporarily point a deployment_status event at a 404 URL, confirm processor logs the skip and posts no PR comment

Acceptance criteria (from #287)

  • Main-document status captured via `Network.responseReceived`
  • Non-2xx aborts capture with a clear error message
  • Unit test covering 200 → screenshot, 404 → throw, 503 → throw
  • Dev smoke (will run after merge once dispatcher chain is settled)

@isadeks isadeks requested a review from a team as a code owner June 8, 2026 18:52
@codecov-commenter

codecov-commenter commented Jun 8, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 96.22642% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@75f1993). Learn more about missing BASE report.

Files with missing lines Patch % Lines
cdk/src/handlers/shared/agentcore-browser.ts 96.22% 2 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #289   +/-   ##
=======================================
  Coverage        ?   86.60%           
=======================================
  Files           ?      185           
  Lines           ?    43494           
  Branches        ?     4360           
=======================================
  Hits            ?    37666           
  Misses          ?     5828           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Closes aws-samples#287.

Before: the processor only checked Page.navigate's errorText. An HTTP
4xx/5xx response or a 3xx redirect that doesn't resolve cleanly
navigates 'successfully' from CDP's perspective, and a 404 / 503 / etc.
PNG gets posted as if it were the deployed app.

After: enable the Network CDP domain before navigating; tap
ws.on('message') for Network.responseReceived events; record the latest
type==='Document' response status for the navigated frame. After
Page.loadEventFired + the 2s settle wait, throw if the captured status
is non-2xx. The processor's existing catch logs and skips the comment
post cleanly.

Auth walls that return 200 (e.g. Vercel deployment protection) are out
of scope — caller-side fix.

Tests: 6 new in agentcore-browser.test.ts:
- 200 → captures as before
- 404 / 503 → throw with status in message
- 301 redirect → throw (defensive)
- non-Document responses (Stylesheet etc.) ignored
- no Network events fire → falls through (pre-aws-samples#287 behaviour)

Architecture-notes update + Starlight mirror sync. Test file is new on
this branch; PR aws-samples#275 (the broader screenshot test PR) doesn't touch
this file.
@isadeks isadeks force-pushed the feat/287-screenshot-network-status branch from 95d942d to 398a1c3 Compare June 10, 2026 19:38
isadeks added 2 commits June 10, 2026 15:51
Closes aws-samples#97.

Adds 53 jest tests across the four screenshot files that landed with
PR aws-samples#241 + aws-samples#273 with no existing coverage:

- github-webhook-verify.test.ts (14): SHA256 sign/verify, sm cache TTL +
  forceRefresh, ResourceNotFound, transparent re-fetch on signature
  mismatch / null fresh.
- github-webhook.test.ts (15): missing body/sig, ping ack, non-deploy
  events ignored, malformed JSON, state/environment filters,
  SCREENSHOT_TARGET_ENVIRONMENT override, missing fields, dedup hit,
  happy path, rollback-on-invoke-failure, non-condition DDB error.
- linear-issue-lookup.test.ts (18): regex covers extract / multi /
  bounds / case-sensitivity, prefix-routing happy path, case-insensitive
  prefix match, fallback for legacy rows + post-prefix-miss, null token
  skip, fuzzy-match guard, GraphQL errors / non-2xx / network failure.
- github-webhook-processor.test.ts (15): empty / malformed body, missing
  fields, token resolve failure, PR retry exhaustion, OPEN-only filter,
  happy path with CloudFront-host URL assertion, screenshot/S3/comment
  failure modes (each non-fatal where appropriate), Linear branch fires
  / falls back to body / skips on no-id / no-resolve / non-fatal post.
- agentcore-browser.test.ts (6): StartBrowserSession failures, full CDP
  exchange (Target.getTargets -> attach -> enable -> navigate ->
  loadEventFired -> captureScreenshot) returning PNG bytes, Stop
  invoked in finally even on CDP error, Stop's own failure logged not
  thrown, 403 unexpected-response surfaced, navigate errorText raised.

All tests use jest mocks for AWS SDK clients + an in-test FakeWebSocket
for the CDP stream so they run hermetically without real AWS or
network. Existing 286/286 handler tests still pass.
…les#240 API

Updates captureScreenshot budget-arg + high-entropy S3 key assertions
to match main's aws-samples#240 versions, plus eslint formatting.
@isadeks

isadeks commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main now that #241 has merged — the duplicated pipeline commits drop out, so this PR's net diff is just its own feature (the non-2xx skip) plus the screenshot-pipeline test coverage absorbed from #275 (which I'm closing as superseded — see below).

Net diff vs main:

#275's other two test files (github-webhook-verify, linear-issue-lookup) were not carried over: #240 already shipped a superset of the verify tests on main, and the richer linear-issue-lookup tests belong to #273 (prefix-routing), not here. All 54 tests green locally.

@krokoko krokoko added this pull request to the merge queue Jun 10, 2026
Merged via the queue into aws-samples:main with commit af33bc0 Jun 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(screenshot): skip screenshot when preview URL returns non-2xx

3 participants